Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add web-time #907

Closed
wants to merge 3 commits into from
Closed

Conversation

misssonder
Copy link
Contributor

according to #891, since instant is no longer maintained, we should use web-time to replace it. Actually I have built WASM to make sure it works in it, but it seems that I don't find unit tests for WASM.

@schungx
Copy link
Collaborator

schungx commented Aug 5, 2024

It is failing the no-std build... Can you check?

Also I wonder if it is possible to write tests for WASM...

@misssonder
Copy link
Contributor Author

Also I wonder if it is possible to write tests for WASM...

I'm not sure if I can write tests for WASM, but I'll try it. BTW, I have fixed the no-std build.

@misssonder
Copy link
Contributor Author

If we add web-time with optional = true then we can't pass the cargo build --target wasm32-unknown-unknown --no-default-features compile; Otherwise if we set optional = false, we can't pass cargo build --manifest-path=no_std/no_std_test/Cargo.toml --profile unix compile. It seems like there isn't perfect way to solve it.

@schungx
Copy link
Collaborator

schungx commented Aug 5, 2024

Why is the wasm build failing without default features?

@schungx
Copy link
Collaborator

schungx commented Aug 5, 2024

[target.'cfg(not(feature = "no_time"))'.dependencies]

Why did you change it to no_time instead? Before it is pulled in only for wasm which doesnt have Instant.

@misssonder
Copy link
Contributor Author

[target.'cfg(not(feature = "no_time"))'.dependencies]

Why did you change it to no_time instead? Before it is pulled in only for wasm which doesnt have Instant.

You are correct. I have changed it to target.'cfg(all(target_family = "wasm", not(feature = "no_time")))'.dependencies, but it still fails the tests. It appears that when we add web-time as dependencies, it fails the no_std tests. I'm not certain if it's because of the implementation of web-time.

@schungx
Copy link
Collaborator

schungx commented Aug 9, 2024

It shoudnt... no-std is not supposed to pull in web-time, then it shouldnt be affected.

If it still fails that means web-time is still being brought in somehow...

@misssonder
Copy link
Contributor Author

I'm pretty sure the issue is related to once_cell and web_time. I've created an example to reproduce the problem. Can run cargo c --features time or cargo c to see the issue. However, it works correctly if I switch from web_time to instant. I'm not sure why this is happening.

@schungx
Copy link
Collaborator

schungx commented Aug 20, 2024

Is it possible that web-time does not support no-std?

@misssonder
Copy link
Contributor Author

It's not supported to be pulled if I don't add features time in this, but it still failed.

@schungx
Copy link
Collaborator

schungx commented Aug 21, 2024

It's not supported to be pulled if I don't add features time in this, but it still failed.

If only one dependency crate in the entire tree depends on std, then stdlib will be pulled in, causing compilation error in no-std.

@misssonder
Copy link
Contributor Author

It turn out that feature = ... in target.'cfg(...)'.dependencies is not supported for selecting dependencies and will not work as expected. So actually rhai will always pull instant whenever it's wasm or not. But web-time don't support to work in no_std environment, so now we can't use web_time to replace instant.

@misssonder misssonder closed this Aug 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants